return None for lyrics if Tekstowo fails to extract lyrics #3994
Conversation
I experienced a failure to parse Tekstowo for song lyrics. This patch allowed the lyrics plugin to fetch the lyrics from another provider as opposed to failing.
There was a problem hiding this comment.
Looks good to me; see the inline comment for a minor improvement to make. If you'd like you could also add your credits to the changelog at docs/changelog.rst.
No bug was reported before this patch was generated
That's fine! As mentioned in the other pull request, it would however be great if you could briefly explain in the pull request how and why the code used to be wrong, as opposed to only saying that it failed. That would help a lot with review.
| @@ -483,7 +483,10 @@ def extract_lyrics(self, html): | |||
| if not soup: | |||
| return None | |||
|
|
|||
| return soup.find("div", class_="song-text").get_text() | |||
| c = soup.find("div", class_="song-text") | |||
There was a problem hiding this comment.
Small nitpick: Better rename this variable to something meaningful, e.g. lyrics_div (which is a name also used by another lyrics source).
|
As a side note, the tekstowo provider appears to have more issues of this kind: Lines 463 to 465 in efcc5b3 will crash as soon as the website is changed in such a way that this tag hierarchy is modified one of the |
|
Hmm, that's a good point, @wisp3rwind—maybe we were a little hasty with enabling this source by default and would be better off leaving it as optional until it's better tested? |
Actually, the |
|
Thanks @matlads for pointing out this issue 🚀 |
Description
I experienced a failure to parse Tekstowo for song lyrics.
This patch allowed the lyrics plugin to fetch the lyrics from another provider as opposed to failing.
No bug was reported before this patch was generated